-
Notifications
You must be signed in to change notification settings - Fork 3.6k
HHH-19365 GaussDB Dialect Support #10199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Everything is ready for review |
Build and test:
BUILD SUCCESSFUL in 25m 56s |
IMHO, the GaussDB Dialect should extends PostgreSQL Dialect to reuse code as possible, not to duplicate all related sources. |
Actually, GaussDB has many different features than PostgreSQL and now the tested mode is Oracle compatibility . In future, GaussDB will not compatible with PostgreSQL but with OpenGauss. And a separation of code may help us testing and add new features. Your suggestion is quite good for less code duplication. But there are some potential risks. For example, a contributor modified PostgreSQL related code will influence GaussDB, however, Hibernate community is not respnosible for fixing problems in community dialect. |
...ialects/src/main/java/org/hibernate/community/dialect/GaussDBAbstractStructuredJdbcType.java
Fixed
Show fixed
Hide fixed
...ialects/src/main/java/org/hibernate/community/dialect/GaussDBAbstractStructuredJdbcType.java
Fixed
Show fixed
Hide fixed
...ialects/src/main/java/org/hibernate/community/dialect/GaussDBAbstractStructuredJdbcType.java
Fixed
Show resolved
Hide resolved
...ialects/src/main/java/org/hibernate/community/dialect/GaussDBAbstractStructuredJdbcType.java
Fixed
Show fixed
Hide fixed
...ialects/src/main/java/org/hibernate/community/dialect/GaussDBAbstractStructuredJdbcType.java
Fixed
Show fixed
Hide fixed
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/GaussDBDialect.java
Fixed
Show fixed
Hide fixed
hibernate-community-dialects/src/main/java/org/hibernate/community/dialect/GaussDBDialect.java
Fixed
Show resolved
Hide resolved
...ialects/src/main/java/org/hibernate/community/dialect/aggregate/GaussDBAggregateSupport.java
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, GaussDB has many different features than PostgreSQL and now the tested mode is Oracle compatibility . In future, GaussDB will not compatible with PostgreSQL but with OpenGauss. And a separation of code may help us testing and add new features.
Your suggestion is quite good for less code duplication. But there are some potential risks. For example, a contributor modified PostgreSQL related code will influence GaussDB, however, Hibernate community is not respnosible for fixing problems in community dialect.
This amount of code copying is IMO neither sensible nor useful. Please reuse as much of the code as possible and start copying + adapting once you GaussDB becomes non-compatible to PostgreSQL.
...-dialects/src/main/java/org/hibernate/community/dialect/GaussDBCallableStatementSupport.java
Outdated
Show resolved
Hide resolved
You have mentioned two options: 1. GaussDB reuse code of PostgreSQL and 2. Not testging GaussDB in CI. If I follow option 1, and I think I can't follow option 2, because It's hard for me to know when to test the code(Maybe every minor release of hibernate). If I follow optioin 2, I can't follow option 1, a seperation of code can avoid side effects from modifications to PostgreSQL. |
@liubao68 I think you're thinking that copying a lot of code will make your code more resistant to breakage when the rest of HIbernate changes, but in fact it seems to me much more likely that the exact opposite is true. The less copied code you are responsible for maintaining, the more you will benefit from fixes everyone else makes. |
Thank you for your suggestion. I will take more time to analyse the code copied from PostgreSQL implementation and check their differences and make a final decision. Actuall many classes have some differences that I can't extend from PostgreSQL classes. I just don't want to dependend on PostgreSQL implementations, and I will reuse hibernate codes as much as possible. And in my future plan, I will add |
You can e.g. sync your fork automatically and add a custom workflow file to the fork for testing GaussDB on GitHub Actions of your fork. That way, you get the latest changes built and tested automatically and if you subscribe to your fork, you will receive emails about workflow failures so you can act on that. |
About code
Thanks. I will try it. |
There is one major problem left about code duplication, I'd like give more explaination about it. Because we developed a new Dialect, for best practises, we reused PostgreSQL's code and structures to help us do it easier. And I compared files newly added which reused from PostgreSQL, almost one harf is different from PostgreSQL and cann't inheritant in class level like
Besides from these identical files, there are same other newly added files PostgreSQL do not have. And most of all, I will give an example that's not quite appropriate. The OracleXmlArrayJdbcTypeConstructor is quite identical to the PostgreSQLArrayJdbcTypeConstructor in code. If OracleXmlArrayJdbcTypeConstructor extends from PostgreSQLArrayJdbcTypeConstructor, it would imply that Oracle has compatibility considerations with PostgreSQL, which could mislead new developers. In its early days, GaussDB was developed from PostgreSQL, but it is no longer compatible with PostgreSQL in the present and future. I do not expect new developers to have such a misunderstanding about compatibility. |
I add a fored push to resolve conflicts. |
Please, just try to reuse as much code as possible instead of copying code. For example, apparently you implement the same string format for structures/array like PostgreSQL since |
Do you mean |
End-users shouldn't interact with these types directly, so I hardly think that when you extend these types, it will imply anything. I understand your point, but you can still copy the code when you realize that it doesn't work anymore for what GaussDB needs. |
To be very clear, I'm not talking about reusing such glue classes like |
Thanks. I got it. I will check these classes. And I remember only AbstractPostgreSQLStructJdbcType. I will check all of them later. |
As discussed, move new Dialect Support to community.
see old PR to core #10093
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
https://hibernate.atlassian.net/browse/HHH-19365